Skip to content

sort: Fix inconsistent sort orderg under i18n-collator with equal sorting keys.#12013

Merged
cakebaker merged 11 commits intouutils:mainfrom
ksgk1:sort-fix-inconsistent-sort-behaviour-i18n-collator
Apr 30, 2026
Merged

sort: Fix inconsistent sort orderg under i18n-collator with equal sorting keys.#12013
cakebaker merged 11 commits intouutils:mainfrom
ksgk1:sort-fix-inconsistent-sort-behaviour-i18n-collator

Conversation

@ksgk1
Copy link
Copy Markdown
Contributor

@ksgk1 ksgk1 commented Apr 26, 2026 •
edited
Loading

In #11980 it was noted, that the sorting behaviour was inconsistent. The cause for this was equal sorting keys, without a fallback option.

I implemented a fallback for equal keys, that fixes this issue.

@ksgk1 ksgk1 mentioned this pull request Apr 26, 2026
Closed
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 26, 2026 •
edited
Loading

GNU testsuite comparison:

GNU test failed: tests/misc/sync. tests/misc/sync is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/pr/bounded-memory (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/tail-n0f (passes in this run but fails in the 'main' branch)

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 26, 2026 •
edited
Loading

Merging this PR will not alter performance

✅ 311 untouched benchmarks
⏩ 46 skipped benchmarks1

Comparing ksgk1:sort-fix-inconsistent-sort-behaviour-i18n-collator (f7b05d8) with main (5316f58)

Open in CodSpeed

Footnotes

  1. 46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@cakebaker cakebaker linked an issue Apr 27, 2026 that may be closed by this pull request
Inconsistent sort behavior #11980
Closed
@cakebaker
Copy link
Copy Markdown
Contributor

cakebaker commented Apr 27, 2026

Can you please add a test to tests/by-util/test_sort.rs to ensure we don't regress in the future? Thanks.

@ksgk1
Copy link
Copy Markdown
Contributor Author

ksgk1 commented Apr 27, 2026

I updated my code, and added different test cases.

cakebaker
cakebaker reviewed Apr 28, 2026
View reviewed changes
Comment thread tests/by-util/test_sort.rs Outdated
@ksgk1 @cakebaker
Update tests/by-util/test_sort.rs
7fcd0e3 
Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
cakebaker
cakebaker reviewed Apr 28, 2026
View reviewed changes
Comment thread tests/by-util/test_sort.rs Outdated
Comment on lines +2967 to +2973
let expected_output = "01\n01\n0_1\n0_1\n02\n02\n";
new_ucmd!()
.env("LC_ALL", "en_US.UTF-8")
.arg("fix_i18n_collate_inconsistency_1.txt")
.arg("fix_i18n_collate_inconsistency_2.txt")
.succeeds()
.stdout_is(expected_output);
Copy link
Copy Markdown
Contributor

@cakebaker cakebaker Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is not correct as GNU sort returns a different result when running the command manually:

$ LC_ALL=en_US.UTF-8 sort tests/fixtures/sort/fix_i18n_collate_inconsistency_1.txt tests/fixtures/sort/fix_i18n_collate_inconsistency_2.txt 
0_1
0_1
01
01
02
02

cakebaker
cakebaker reviewed Apr 28, 2026
View reviewed changes
Comment thread tests/by-util/test_sort.rs
@ksgk1
Copy link
Copy Markdown
Contributor Author

ksgk1 commented Apr 28, 2026

Thank you a lot for the valuable feedback. I will fix those issues as soon as possible and verify my implementation against sort's output.

@ksgk1
Copy link
Copy Markdown
Contributor Author

ksgk1 commented Apr 29, 2026

I have looked into the test cases again and removed one redundant one. I ensured that in both tests, I match sort's original output for the same files:

LC_ALL=en_US.UTF-8 sort tests/fixtures/sort/fix_i18n_collate_inconsistency_1.txt tests/fixtures/sort/fix_i18n_collate_inconsistency_2.txt
0_1
0_1
01
01
02
02

and

LC_ALL=C sort tests/fixtures/sort/fix_i18n_collate_inconsistency_1.txt tests/fixtures/sort/fix_i18n_collate_inconsistency_2.txt
01
01
02
02
0_1
0_1

@cakebaker cakebaker merged commit b3d8706 into uutils:main Apr 30, 2026
170 of 173 checks passed
@cakebaker
Copy link
Copy Markdown
Contributor

cakebaker commented Apr 30, 2026

Thanks for your PR!

@ksgk1 ksgk1 deleted the sort-fix-inconsistent-sort-behaviour-i18n-collator branch April 30, 2026 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cakebaker cakebaker cakebaker left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent sort behavior

2 participants

@ksgk1 @cakebaker